Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add typescript support 🎉 #42

Merged
merged 4 commits into from
Feb 11, 2023
Merged

Add typescript support 🎉 #42

merged 4 commits into from
Feb 11, 2023

Conversation

benwinding
Copy link
Owner

Summary

  • This adds Typescript to the project, as it's getting quite large and hard to understand in pure JS
  • Should reduce bugs and make it a bit nicer to work on

Hi @rouilj
I still appreciate all the attention to this repo. I'm going to review all your comments & PR's in the next few days 👍

@rouilj
Copy link
Contributor

rouilj commented Feb 11, 2023

eek. ok. I am barely fluent in js never mind typescript.

@benwinding
Copy link
Owner Author

eek. ok. I am barely fluent in js never mind typescript.

Hopefully it's much nicer, you get way more help in the IDE

@benwinding benwinding merged commit 578f45f into master Feb 11, 2023
@benwinding benwinding deleted the add-typescript branch February 11, 2023 00:58
@rouilj
Copy link
Contributor

rouilj commented Feb 11, 2023

Well I use emacs so....

@benwinding
Copy link
Owner Author

Well I use emacs so....

Oh yeah sorry, well at least compile time errors will show up for you 😅
Or run npm run check-types in another terminal and you'll get type errors there 🙏

@rouilj
Copy link
Contributor

rouilj commented Feb 11, 2023

Thanks. Merging master to my merged branch (that has all the changes merged together)
broke things for a while till I reran npm run dev and got the newer files although I am still
getting some errors on a particular version of some packages.

However, I can run dev and run build and it seems to work so....

@benwinding
Copy link
Owner Author

although I am still getting some errors on a particular version of some packages.

That was breaking CI too, it might be fixed by 533c583

@rouilj
Copy link
Contributor

rouilj commented Feb 11, 2023 •
edited
Loading

I have that commit. It's still having issues.

Also svelte is breaking on npm run check-types on the a?.b construct.

/home/rouilj/develop/command-pal/node_modules/svelte-check/dist/src/index.js:5766
		        return this._head?.value;
		                          ^

SyntaxError: Unexpected token '.'
    at wrapSafe (internal/modules/cjs/loader.js:915:16)
    at Module._compile (internal/modules/cjs/loader.js:963:27)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:863:32)
  ...

dpkg -l reports:

nodejs                                12.22.9~dfsg-1ubuntu3

which is the version with ubuntu 22.04.

@benwinding
Copy link
Owner Author

Thanks for the info, it looks like svelte-preprocess requires node 14+

image

I use node 16 which is pretty standard now (Github's CI tool now only uses node 16)

Are you using nvm? I highly recommend it, it's easy, and works really well on Ubuntu too

image

@rouilj
Copy link
Contributor

rouilj commented Feb 11, 2023

Well that broke everything.

$ node --version
v16.19.0

$ npm i rollup-plugin-svelte
npm ERR! code ERESOLVE
npm ERR! ERESOLVE could not resolve
npm ERR! 
npm ERR! While resolving: @rollup/[email protected]
npm ERR! Found: [email protected]
npm ERR! node_modules/rollup
npm ERR!   peer rollup@"^1.20.0||^2.0.0" from @rollup/[email protected]
npm ERR!   node_modules/@rollup/plugin-node-resolve
npm ERR!     dev @rollup/plugin-node-resolve@"^7.0.0" from the root project
npm ERR!   peer rollup@"^1.20.0||^2.0.0" from @rollup/[email protected]
npm ERR!   node_modules/@rollup/pluginutils
npm ERR!     @rollup/pluginutils@"^3.0.0" from @rollup/[email protected]
npm ERR!     node_modules/@rollup/plugin-commonjs
npm ERR!       dev @rollup/plugin-commonjs@"11.x" from the root project
npm ERR!     @rollup/pluginutils@"^3.0.8" from @rollup/[email protected]
npm ERR!     node_modules/@rollup/plugin-node-resolve
npm ERR!       dev @rollup/plugin-node-resolve@"^7.0.0" from the root project
npm ERR!   3 more (rollup-plugin-terser, the root project, rollup-plugin-svelte)
npm ERR! 
npm ERR! Could not resolve dependency:
npm ERR! peer rollup@"^1.20.0" from @rollup/[email protected]
npm ERR! node_modules/@rollup/plugin-commonjs
npm ERR!   dev @rollup/plugin-commonjs@"11.x" from the root project
npm ERR! 
npm ERR! Conflicting peer dependency: [email protected]
npm ERR! node_modules/rollup
npm ERR!   peer rollup@"^1.20.0" from @rollup/[email protected]
npm ERR!   node_modules/@rollup/plugin-commonjs
npm ERR!     dev @rollup/plugin-commonjs@"11.x" from the root project
npm ERR! 
npm ERR! Fix the upstream dependency conflict, or retry

This probably needs to be part of #15 8-)

I'm taking this as a reason to stop working on my next patches for tonight 8-).

@rouilj
Copy link
Contributor

rouilj commented Feb 11, 2023

Ok, I failed to follow my own advice. Did a new clone in a new directory and I am working again.

32 vulnerabilities (14 moderate, 17 high, 1 critical)

@benwinding
Copy link
Owner Author

Generally in node development, rm -rf ./node_modules is required, when switching node versions or major dependency changes in the package.json (i.e. merging a branch in or something)

@rouilj
Copy link
Contributor

rouilj commented Feb 13, 2023

Sign, yeah I figured that out finally and now I really do have a working install 8-/.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@benwinding @rouilj